feat: add support for bucket encryption enforcement config#1742
feat: add support for bucket encryption enforcement config#1742rajeevpodar wants to merge 4 commits intomainfrom
Conversation
This change introduces support for configuring bucket encryption enforcement, including: - `defaultKmsKeyName` (via `BucketEncryption`) - `googleManagedEncryptionEnforcementConfig` - `customerManagedEncryptionEnforcementConfig` - `customerSuppliedEncryptionEnforcementConfig` New classes `EncryptionEnforcementConfig` and `BucketEncryption` are added to `google/cloud/storage/bucket.py` to wrap the API configuration. The `Bucket` class now exposes an `encryption` property. Tests are added in `tests/unit/test_bucket.py`. Co-authored-by: rajeevpodar <3637722+rajeevpodar@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @rajeevpodar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Google Cloud Storage client library by introducing robust support for managing bucket encryption enforcement configurations. It provides a structured API for users to define and apply various encryption policies, including setting restriction modes and handling different types of encryption (Google-managed, customer-managed, and customer-supplied) directly through the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for bucket encryption enforcement configuration, introducing EncryptionEnforcementConfig and BucketEncryption classes and adding an encryption property to the Bucket class. The changes are well-structured and include corresponding unit tests. I have a few suggestions to improve code quality and test robustness.
| This value should normally only be set by the back-end API. | ||
| """ | ||
|
|
||
| def __init__(self, restriction_mode=None, effective_time=None, **kw): |
There was a problem hiding this comment.
The **kw parameter in the EncryptionEnforcementConfig constructor is unused. Removing it would clean up the method signature and prevent accidental misuse with unsupported arguments.
| def __init__(self, restriction_mode=None, effective_time=None, **kw): | |
| def __init__(self, restriction_mode=None, effective_time=None): |
| def test_restriction_mode_setter(self): | ||
| config = self._make_one() | ||
| self.assertIsNone(config.restriction_mode) | ||
| config.restriction_mode = "FULLY_RESTRICTED" |
There was a problem hiding this comment.
Throughout the new tests, hardcoded strings like "FULLY_RESTRICTED" and "NOT_RESTRICTED" are used. It would be better to use the constants ENFORCEMENT_MODE_FULLY_RESTRICTED and ENFORCEMENT_MODE_NOT_RESTRICTED from google.cloud.storage.constants. This improves maintainability and consistency. This applies here and in Test_BucketEncryption (e.g., lines 4841, 4850, 4859, 4866, 4878, 4883). You'll need to add the necessary import.
| config.restriction_mode = "FULLY_RESTRICTED" | |
| config.restriction_mode = ENFORCEMENT_MODE_FULLY_RESTRICTED |
There was a problem hiding this comment.
@rajeevpodar don't accept this, msged you privately
| encryption.default_kms_key_name = "new-key" | ||
| bucket._patch_property.assert_called_with("encryption", encryption) | ||
|
|
||
| config = EncryptionEnforcementConfig("NOT_RESTRICTED") | ||
| encryption.google_managed_encryption_enforcement_config = config | ||
| bucket._patch_property.assert_called_with("encryption", encryption) | ||
| self.assertEqual( | ||
| encryption.google_managed_encryption_enforcement_config.restriction_mode, | ||
| "NOT_RESTRICTED", | ||
| ) | ||
|
|
||
| encryption.customer_managed_encryption_enforcement_config = config | ||
| bucket._patch_property.assert_called_with("encryption", encryption) | ||
|
|
||
| encryption.customer_supplied_encryption_enforcement_config = config | ||
| bucket._patch_property.assert_called_with("encryption", encryption) |
There was a problem hiding this comment.
The repeated use of assert_called_with only verifies the most recent call to bucket._patch_property. This can make the test brittle. To ensure that each setter triggers a call, it would be more robust to check the call_count after each call, or to use mock.assert_has_calls at the end of the test to verify all expected calls were made.
For example:
bucket = self._make_bucket()
encryption = self._make_one(bucket)
encryption.default_kms_key_name = "new-key"
config = EncryptionEnforcementConfig("NOT_RESTRICTED")
encryption.google_managed_encryption_enforcement_config = config
encryption.customer_managed_encryption_enforcement_config = config
encryption.customer_supplied_encryption_enforcement_config = config
self.assertEqual(bucket._patch_property.call_count, 4)
bucket._patch_property.assert_called_with("encryption", encryption)This change introduces support for configuring bucket encryption enforcement, including: - `defaultKmsKeyName` (via `BucketEncryption`) - `googleManagedEncryptionEnforcementConfig` - `customerManagedEncryptionEnforcementConfig` - `customerSuppliedEncryptionEnforcementConfig` New classes `EncryptionEnforcementConfig` and `BucketEncryption` are added to `google/cloud/storage/bucket.py` to wrap the API configuration. The `Bucket` class now exposes an `encryption` property. Tests are added in `tests/unit/test_bucket.py` and `tests/system/test_bucket.py`. Co-authored-by: rajeevpodar <3637722+rajeevpodar@users.noreply.github.com>
This change introduces support for configuring bucket encryption enforcement, including: - `defaultKmsKeyName` (via `BucketEncryption`) - `googleManagedEncryptionEnforcementConfig` - `customerManagedEncryptionEnforcementConfig` - `customerSuppliedEncryptionEnforcementConfig` New classes `EncryptionEnforcementConfig` and `BucketEncryption` are added to `google/cloud/storage/bucket.py` to wrap the API configuration. The `Bucket` class now exposes an `encryption` property. Tests are added in `tests/unit/test_bucket.py` and `tests/system/test_bucket.py`. Addressed review comments regarding kwargs handling, constants usage in tests, and null safety in getters. Co-authored-by: rajeevpodar <3637722+rajeevpodar@users.noreply.github.com>
This change introduces support for configuring bucket encryption enforcement, including: - `defaultKmsKeyName` (via `BucketEncryption`) - `googleManagedEncryptionEnforcementConfig` - `customerManagedEncryptionEnforcementConfig` - `customerSuppliedEncryptionEnforcementConfig` New classes `EncryptionEnforcementConfig` and `BucketEncryption` are added to `google/cloud/storage/bucket.py` to wrap the API configuration. The `Bucket` class now exposes an `encryption` property. Tests are added in `tests/unit/test_bucket.py` and `tests/system/test_bucket.py`. Addressed review comments: - Handling `**kw` in `EncryptionEnforcementConfig.__init__` for forward compatibility. - Using constants for restriction modes in tests. - Improving test robustness. - Formatting with black. Co-authored-by: rajeevpodar <3637722+rajeevpodar@users.noreply.github.com>
| """ | ||
|
|
||
| ENFORCEMENT_MODE_FULLY_RESTRICTED = "FULLY_RESTRICTED" | ||
| """Bucket encryption restriction mode where encryption is fully restricted.""" |
There was a problem hiding this comment.
nit: comment should be above the variable name.
| :type restriction_mode: str | ||
| :param restriction_mode: | ||
| (Optional) The restriction mode for the encryption type. | ||
| See: https://cloud.google.com/storage/docs/json_api/v1/buckets#encryption |
There was a problem hiding this comment.
nit: docs not yet updated.
| if effective_time is not None: | ||
| data["effectiveTime"] = _datetime_to_rfc3339(effective_time) |
There was a problem hiding this comment.
this is server provided. We should not be mutating this. (I guess, server will ignore this field if it's set in request anyway)
| def test_restriction_mode_setter(self): | ||
| config = self._make_one() | ||
| self.assertIsNone(config.restriction_mode) | ||
| config.restriction_mode = "FULLY_RESTRICTED" |
There was a problem hiding this comment.
@rajeevpodar don't accept this, msged you privately
There was a problem hiding this comment.
lot of tests are deleted. Jules hallucinated ?
Implemented bucket encryption enforcement configuration support.
Added
EncryptionEnforcementConfigandBucketEncryptionclasses.Added
encryptionproperty toBucket.Added unit tests.
PR created automatically by Jules for task 10151670798613550332 started by @rajeevpodar